[REVIEW] Replace function#106
[REVIEW] Replace function#106gcca wants to merge 36 commits intorapidsai:masterfrom BlazingDB:replace-function
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
API seems reasonable to me |
| @@ -0,0 +1,110 @@ | |||
| /* | |||
There was a problem hiding this comment.
This should be moved to a separate bench folder as we discussed in the binary ops PR review?
There was a problem hiding this comment.
And separated into benchmarks and unit tests?
| @@ -0,0 +1,143 @@ | |||
| /* | |||
There was a problem hiding this comment.
Should add more documentation/comments to the tests
There was a problem hiding this comment.
@nsakharnykh Can you approve your review since this is resolved?
…s to replace-test
…s to replace-test
harrism
left a comment
There was a problem hiding this comment.
Consider changing the function name or at least improving the documentation of what it does. One other minor suggestion.
include/gdf/cffi/functions.h
Outdated
There was a problem hiding this comment.
The interface is confusing to me. Why does it have column and to_replace parameters? What happens to column? What happens to to_replace? Why not just make a gdf_copy(out_column, in_column)?
There was a problem hiding this comment.
Now I see. The meaning of "replace" is not clear. It's not a copy. The semantics are actually those of "find_and_replace_all": For each value in to_replace, find all instances of that value in column and replace it with the corresponding value in values. This should be made clear in the header documentation for the function. Consider changing the name to gdf_find_and_replace_all() or something like that...
| @@ -0,0 +1,43 @@ | |||
|
|
|||
There was a problem hiding this comment.
We're adding google benchmark in this PR? Shouldn't that be orthogonal to gdf_replace, and therefore a separate PR?
There was a problem hiding this comment.
Since this is the first benchmark I've seen added to libgdf...
There was a problem hiding this comment.
Since no other PR depends of this, I put google benchmark here. But I could make another PR if it's relevant.
src/replace.cu
Outdated
There was a problem hiding this comment.
I don't really like calling this macro "WHEN" -- it's not descriptive. Something like REPLACE_CASE would be clearer. Then you would have
switch (column->type) {
REPLACE_CASE(INT8);
REPLACE_CASE(INT16);
// etc.
}
API proposal for the replace function.